src/qt/sendcoinsentry.cpp (1)

91-94: Consider adding message field validation.

While the message field functionality is implemented correctly, consider adding:

1. Character length limits to prevent oversized messages
2. Input validation to ensure message content meets any protocol requirements
3. Visual feedback about remaining character count

This would improve user experience and prevent potential issues with transaction creation.

src/qt/forms/sendcoinsentry.ui (3)

234-239: Consider adding input constraints and placeholder text.

The message field would benefit from additional properties:

• Maximum length constraint to prevent oversized messages
• Placeholder text to guide users on the expected input

 <widget class="QLineEdit" name="messageTextLabel">
   <property name="toolTip">
     <string>Optional message for this transaction</string>
   </property>
+  <property name="maxLength">
+    <number>140</number>
+  </property>
+  <property name="placeholderText">
+    <string>Enter a message to include with the transaction</string>
+  </property>
 </widget>

---

Line range hint 729-738: Update tab order to include message field.

The message field (messageTextLabel) should be included in the tab order sequence after the payment amount.

 <tabstops>
  <tabstop>payTo</tabstop>
  <tabstop>addressBookButton</tabstop>
  <tabstop>pasteButton</tabstop>
  <tabstop>deleteButton</tabstop>
  <tabstop>addAsLabel</tabstop>
  <tabstop>payAmount</tabstop>
+ <tabstop>messageTextLabel</tabstop>
  <tabstop>payAmount_is</tabstop>
  <tabstop>deleteButton_is</tabstop>
  <tabstop>payAmount_s</tabstop>
  <tabstop>deleteButton_s</tabstop>
 </tabstops>

---

224-227: Consider harmonizing label text across payment forms.

For consistency, consider using "Memo:" instead of "Message:" to match the label text used in both authenticated and unauthenticated payment request forms.

 <widget class="QLabel" name="messageLabel">
   <property name="text">
-    <string>Message:</string>
+    <string>Memo:</string>
   </property>
   <property name="alignment">
     <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set>
   </property>
 </widget>

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Search for other message field implementations in Qt UI files
# and related C++ files to ensure consistent validation and visibility behavior

# Check for other message field implementations in UI files
echo "Checking UI files for message field implementations..."
fd -e ui | xargs grep -l "messageTextLabel\|messageLabel"

# Check for message validation patterns in C++ files
echo "Checking C++ files for message validation patterns..."
fd -e cpp -e h | xargs grep -l "validateSparkAddress\|messageTextLabel"

#!/bin/bash
# Check the actual implementation of message fields and validation in related files
echo "Checking message field implementations in sendcoinsentry.ui..."
cat src/qt/forms/sendcoinsentry.ui

echo -e "\nChecking validateSparkAddress implementations..."
rg "validateSparkAddress" -A 5

echo -e "\nChecking message visibility patterns..."
rg "setVisible.*message" -A 2

#!/bin/bash
# Description: Check for other transaction message handling implementations
# that might need similar updates.

echo "Checking for transaction message handling in UI components..."
rg -l "mapValue\[\"message\"\]" src/qt/

echo "Checking for memo field usage..."
rg -l "\.memo" src/

echo "Checking for other spark message implementations..."
ast-grep --pattern 'strHTML += $_ + tr("Message") + $_'

#!/bin/bash
# Check the specific implementation in transactiondesc.cpp
echo "Checking transaction message implementations in transactiondesc.cpp..."
rg -A 5 "Message" src/qt/transactiondesc.cpp

# Check wallet model implementation
echo "Checking walletmodel.cpp memo handling..."
rg -A 5 "memo" src/qt/walletmodel.cpp

# Check sparkwallet implementation
echo "Checking sparkwallet.cpp memo handling..."
rg -A 5 "memo" src/spark/sparkwallet.cpp

# Look for any other UI message formatting
echo "Checking for other UI message formatting patterns..."
rg "HtmlEscape.*message" src/qt/

#!/bin/bash
# Description: Verify SendCoinsRecipient struct has message field
# Search for SendCoinsRecipient struct definition
rg -A 10 "struct SendCoinsRecipient"

#!/bin/bash
# Let's try different approaches to find the struct definition
# 1. Search in header files
fd -e h -e hpp | xargs rg -l "SendCoinsRecipient"

# 2. Search for any mention of SendCoinsRecipient
rg "SendCoinsRecipient" -A 5 -B 5

# 3. Search specifically in Qt-related directories
fd -e h -e hpp -p ".*qt.*" | xargs rg "SendCoinsRecipient"

src/qt/sendcoinsentry.h (2)

66-66: Add documentation for the new slot method.

The new slot method looks good and follows Qt conventions. However, consider adding a brief documentation comment explaining its purpose and the 256-character limit mentioned in the implementation.

+    /** Handles memo text changes and enforces the 256-character limit */
     void on_MemoTextChanged(const QString &text);

---

66-66: Consider consistent naming between UI and code.

The slot name uses "MemoText" while the UI element is named "messageTextLabel". Consider aligning these names for better code maintainability.

Choose one of these alternatives:

-    void on_MemoTextChanged(const QString &text);
+    void on_messageTextChanged(const QString &text);

or update the UI element name to match:

- messageTextLabel → memoTextEdit

src/qt/sendcoinsentry.cpp (3)

32-32: Consider grouping related UI element initializations.

The message-related UI element initializations are split between the icon setup and visibility settings. Consider grouping them together for better code organization.

 QIcon icon_;
 icon_.addFile(QString::fromUtf8(":/icons/ic_warning"), QSize(), QIcon::Normal, QIcon::On);
 ui->iconWarning->setPixmap(icon_.pixmap(18, 18));
 ui->iconMessageWarning->setPixmap(icon_.pixmap(18, 18));
+
+// Initialize message-related UI elements
+ui->messageLabel->setVisible(false);
+ui->messageTextLabel->setVisible(false);
+ui->iconMessageWarning->setVisible(false);

 ui->addressBookButton->setIcon(platformStyle->SingleColorIcon(":/icons/address-book"));
-
-ui->messageLabel->setVisible(false);
-ui->messageTextLabel->setVisible(false);
-ui->iconMessageWarning->setVisible(false);

Also applies to: 61-63

---

71-87: Improve code quality and maintainability of the memo text handler.

Several improvements can be made to enhance the code:

1. Define the character limit as a constant
2. Use early return pattern
3. Extract UI state management into a helper method

+// At class level
+static const int MAX_MEMO_LENGTH = 256;
+
 void SendCoinsEntry::on_MemoTextChanged(const QString &text)
 {
-    int maxLength = 256;
-    bool isOverLimit = text.length() > maxLength;
+    bool isOverLimit = text.length() > MAX_MEMO_LENGTH;
+    setMemoWarningState(isOverLimit);
+}
 
-    if (isOverLimit) {
-        ui->messageWarning->setText("Message exceeds character 256 character limit");
-        ui->messageWarning->setVisible(true);
-        ui->messageTextLabel->setStyleSheet("border: 1px solid red;");
-        ui->iconMessageWarning->setVisible(true);
-    } else {
-        ui->messageWarning->clear();
-        ui->messageWarning->setVisible(false);
-        ui->messageTextLabel->setStyleSheet("");
-        ui->iconMessageWarning->setVisible(false);
-    }
+private:
+void SendCoinsEntry::setMemoWarningState(bool isOverLimit)
+{
+    ui->messageWarning->setText(isOverLimit ? 
+        QString("Message exceeds character %1 character limit").arg(MAX_MEMO_LENGTH) : 
+        QString());
+    ui->messageWarning->setVisible(isOverLimit);
+    ui->messageTextLabel->setStyleSheet(isOverLimit ? "border: 1px solid red;" : "");
+    ui->iconMessageWarning->setVisible(isOverLimit);
 }

🧰 Tools

🪛 cppcheck

[performance] 80-80: Function parameter 'label' should be passed by const reference.

(passedByValue)

---

[performance] 74-74: Function parameter 'hash_bind_inner' should be passed by const reference.

(passedByValue)

---

[performance] 83-83: Prefer prefix ++/-- operators for non-primitive types.

(postfixOperator)

---

113-118: Improve readability of address validation code.

The implementation correctly handles the null check for the model. Consider extracting the visibility logic into a well-named constant for better readability.

 bool isSparkAddress = false;
 if (model) {
     isSparkAddress = model->validateSparkAddress(address);
 }
-ui->messageLabel->setVisible(isSparkAddress);
-ui->messageTextLabel->setVisible(isSparkAddress);
+const bool shouldShowMessageFields = isSparkAddress;
+ui->messageLabel->setVisible(shouldShowMessageFields);
+ui->messageTextLabel->setVisible(shouldShowMessageFields);

src/qt/forms/sendcoinsentry.ui (2)

Line range hint 224-240: Consider adding input constraints and placeholder text.

The message input field would benefit from additional properties:

1. Add a maximum length constraint to prevent oversized messages
2. Include placeholder text to guide users on the expected input

 <widget class="QLineEdit" name="messageTextLabel">
   <property name="toolTip">
     <string>Optional message for this transaction</string>
   </property>
+  <property name="maxLength">
+    <number>140</number>
+  </property>
+  <property name="placeholderText">
+    <string>Enter an optional message to include with the transaction</string>
+  </property>
 </widget>

---

Line range hint 224-281: Add accessibility properties for screen readers.

Consider adding accessibility properties to improve usability for users with screen readers:

 <widget class="QLineEdit" name="messageTextLabel">
   <property name="toolTip">
     <string>Optional message for this transaction</string>
   </property>
+  <property name="accessibleName">
+    <string>Transaction message input</string>
+  </property>
+  <property name="accessibleDescription">
+    <string>Enter an optional message to be included with this transaction</string>
+  </property>
 </widget>

 <widget class="QLabel" name="iconMessageWarning">
+  <property name="accessibleName">
+    <string>Message warning icon</string>
+  </property>
 </widget>

 <widget class="QLabel" name="messageWarning">
+  <property name="accessibleName">
+    <string>Message warning text</string>
+  </property>
 </widget>

src/qt/sendcoinsdialog.cpp (1)

536-547: Improve message section display in confirmation dialog.

While the implementation properly sanitizes messages using HtmlEscape, consider these improvements:

1. Only show the "Messages" section header if there are actual messages to display
2. Add length limits to prevent dialog layout issues with very long messages

Apply this diff to improve the implementation:

 bool firstMessage = true;
+bool hasMessages = false;
+
+// First check if we have any messages
+for (const auto& rec : recipients) {
+    if (!rec.message.isEmpty()) {
+        hasMessages = true;
+        break;
+    }
+}
+
+if (hasMessages) {
+    questionString.append("<hr><b>" + tr("Messages") + ":</b><br>");
+}

 for (const auto& rec : recipients) {
     if (!rec.message.isEmpty()) {
-        if (firstMessage) {
-            questionString.append("<hr><b>" + tr("Messages") + ":</b><br>");
-            firstMessage = false;
-        }
         QString sanitizedMsg = GUIUtil::HtmlEscape(rec.message, true);
+        // Limit message length for display
+        if (sanitizedMsg.length() > 500) {
+            sanitizedMsg = sanitizedMsg.left(500) + "...";
+        }
         questionString.append("• " + sanitizedMsg + "<br>");
     }
 }

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Search for version handling in serialization
rg -B 5 -A 5 "SerializationOp.*version|SERIALIZE.*version" 

#!/bin/bash
# Search for UI components using CSparkOutputTx
rg -l "CSparkOutputTx.*memo|memo.*CSparkOutputTx" --type cpp --type h

# Search for related UI files
fd -e ui | xargs rg -l "memo"

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: levonpetrosyan93
PR: firoorg/firo#1493
File: src/spark/sparkwallet.cpp:745-746
Timestamp: 2024-11-10T23:59:00.992Z
Learning: When validating memo fields in Spark transactions, use `get_memo_bytes()` from the Spark parameters to get the maximum allowed memo size.

src/qt/sendcoinsentry.cpp (1)

72-72: Add function documentation.

Add a documentation block explaining the purpose and behavior of this function.

+/**
+ * Handles memo text changes, validates length against memo size limit,
+ * sanitizes input, and updates UI feedback accordingly.
+ * @param text The new memo text
+ */
 void SendCoinsEntry::on_MemoTextChanged(const QString &text)

Configuration used: CodeRabbit UI
Review profile: CHILL

Learnt from: levonpetrosyan93
PR: firoorg/firo#1493
File: src/spark/sparkwallet.cpp:745-746
Timestamp: 2024-11-10T23:59:00.992Z
Learning: When validating memo fields in Spark transactions, use `get_memo_bytes()` from the Spark parameters to get the maximum allowed memo size.